feat: Add DocTypeTable component for Issue #75#144
Open
Akshat-0001 wants to merge 5 commits intortCamp:mainfrom
Open
feat: Add DocTypeTable component for Issue #75#144Akshat-0001 wants to merge 5 commits intortCamp:mainfrom
Akshat-0001 wants to merge 5 commits intortCamp:mainfrom
Conversation
- Compose useFrappeGetDocList hook with ListView component - All parameters transparent through DocTypeTable - Support loading, error, and empty states - Custom component overrides - Full TypeScript support - Comprehensive Storybook documentation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR introduces a DocTypeTable component that combines the useFrappeGetDocList hook with the existing ListView component to provide seamless integration with Frappe backend for displaying DocType document lists.
Changes:
- Added
useFrappeGetDocListhook for fetching Frappe document lists with automatic data fetching and state management - Created
DocTypeTablecomponent with support for loading, error, and empty states, along with customizable components and callbacks - Added Storybook stories (though they currently demonstrate ListView rather than DocTypeTable)
- Included comprehensive README documentation with usage examples
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/frappe-ui-react/src/components/hooks/useFrappeGetDocList.ts | New hook for fetching Frappe document lists with transparent parameter passing |
| packages/frappe-ui-react/src/components/hooks/index.ts | Export useFrappeGetDocList hook |
| packages/frappe-ui-react/src/components/docTypeTable/docTypeTable.tsx | Main component implementation with state management and UI rendering |
| packages/frappe-ui-react/src/components/docTypeTable/index.ts | Export DocTypeTable component and types |
| packages/frappe-ui-react/src/components/docTypeTable/docTypeTable.stories.tsx | Storybook stories for component demonstration |
| packages/frappe-ui-react/src/components/docTypeTable/README.md | Comprehensive documentation with examples |
| packages/frappe-ui-react/src/components/index.ts | Export docTypeTable from main components index |
| // Error is handled in the resource state | ||
| }); | ||
| } | ||
| }, [auto, params.doctype, resource]); |
There was a problem hiding this comment.
The useEffect hook has a dependency on resource which is created by useResource. This will cause an infinite loop because resource is a new object on every render. The effect should depend on the actual parameters that affect fetching (like params.doctype, params.fields, params.filters, etc.) or use a JSON.stringify comparison, or the dependencies should be restructured to avoid the infinite loop.
Suggested change
| }, [auto, params.doctype, resource]); | |
| }, [auto, params.doctype]); |
Comment on lines
+43
to
+85
| const buildParams = () => { | ||
| const queryParams = new URLSearchParams(); | ||
|
|
||
| if (params.fields) { | ||
| queryParams.append("fields", JSON.stringify(params.fields)); | ||
| } | ||
|
|
||
| if (params.filters) { | ||
| queryParams.append("filters", JSON.stringify(params.filters)); | ||
| } | ||
|
|
||
| if (params.orderBy) { | ||
| queryParams.append("order_by", params.orderBy); | ||
| } | ||
|
|
||
| // Use limit as pageLength if not specified | ||
| const limit = params.limit || params.pageLength || params.pageSize || 20; | ||
| queryParams.append("limit_page_length", String(limit)); | ||
|
|
||
| if (params.offset) { | ||
| queryParams.append("limit_page_length_offset", String(params.offset)); | ||
| } | ||
|
|
||
| // Add any additional parameters | ||
| Object.keys(params).forEach((key) => { | ||
| if ( | ||
| ![ | ||
| "doctype", | ||
| "fields", | ||
| "filters", | ||
| "orderBy", | ||
| "limit", | ||
| "offset", | ||
| "pageLength", | ||
| "pageSize", | ||
| ].includes(key) | ||
| ) { | ||
| queryParams.append(key, String((params as Record<string, unknown>)[key])); | ||
| } | ||
| }); | ||
|
|
||
| return queryParams.toString(); | ||
| }; |
There was a problem hiding this comment.
The buildParams function is recreated on every render because it's defined inside the component. This will cause the fetchDocList function to be recreated on every render as well, which can lead to unnecessary re-fetching. Consider using useCallback or moving buildParams logic directly into fetchDocList.
| <p className="text-sm text-gray-500 mt-1">{error?.message}</p> | ||
| <button | ||
| onClick={() => fetch()} | ||
| className="mt-3 px-3 py-1 bg-blue-600 text-white rounded hover:bg-blue-700 text-sm" |
There was a problem hiding this comment.
The retry button lacks proper accessibility attributes. Consider adding aria-label to provide a more descriptive label for screen readers, such as "Retry loading data".
Suggested change
| className="mt-3 px-3 py-1 bg-blue-600 text-white rounded hover:bg-blue-700 text-sm" | |
| className="mt-3 px-3 py-1 bg-blue-600 text-white rounded hover:bg-blue-700 text-sm" | |
| aria-label={`Retry loading ${doctype} data`} |
packages/frappe-ui-react/src/components/hooks/useFrappeGetDocList.ts
Outdated
Show resolved
Hide resolved
packages/frappe-ui-react/src/components/docTypeTable/docTypeTable.tsx
Outdated
Show resolved
Hide resolved
packages/frappe-ui-react/src/components/docTypeTable/docTypeTable.tsx
Outdated
Show resolved
Hide resolved
Comment on lines
+116
to
+132
| <div className="flex items-center justify-center py-8 text-red-600" {...attrs}> | ||
| {typeof errorComponent === "function" | ||
| ? errorComponent(error) | ||
| : errorComponent || ( | ||
| <div className="text-center"> | ||
| <p className="font-medium">Error loading {doctype}</p> | ||
| <p className="text-sm text-gray-500 mt-1">{error?.message}</p> | ||
| <button | ||
| onClick={() => fetch()} | ||
| className="mt-3 px-3 py-1 bg-blue-600 text-white rounded hover:bg-blue-700 text-sm" | ||
| > | ||
| Retry | ||
| </button> | ||
| </div> | ||
| )} | ||
| </div> | ||
| ); |
There was a problem hiding this comment.
The error state div lacks proper ARIA attributes. Consider adding role="alert" to announce errors to screen readers immediately.
Comment on lines
+1
to
+234
| import type { Meta, StoryObj } from "@storybook/react-vite"; | ||
| import { Avatar } from "../avatar"; | ||
| import ListView from "../listview/listView"; | ||
|
|
||
| const meta: Meta = { | ||
| title: "Components/DocTypeTable", | ||
| parameters: { | ||
| layout: "padded", | ||
| docs: { | ||
| description: { | ||
| component: ` | ||
| DocTypeTable is a general-purpose table component for displaying Frappe DocType documents. | ||
|
|
||
| ## Key Features | ||
| - **Automatic Data Fetching**: Fetches data from Frappe backend using useFrappeGetDocList hook | ||
| - **Transparent Parameter Passing**: All useFrappeGetDocList parameters flow directly through the component | ||
| - **Configurable Columns**: Define table structure with custom rendering | ||
| - **State Management**: Handles loading, error, and empty states | ||
| - **Selection Support**: Built-in row selection capability | ||
| - **Customizable Components**: Override loading, error, and empty states with custom components | ||
|
|
||
| ## Usage Example | ||
| \`\`\`tsx | ||
| <DocTypeTable | ||
| doctype="User" | ||
| columns={[ | ||
| { | ||
| label: "Name", | ||
| key: "name", | ||
| width: 3, | ||
| prefix: ({ row }) => <Avatar image={row.user_image} label={row.name} /> | ||
| }, | ||
| { label: "Email", key: "email", width: "200px" }, | ||
| { label: "Type", key: "user_type" }, | ||
| ]} | ||
| params={{ | ||
| fields: ['name', 'email', 'user_type', 'enabled'], | ||
| filters: { enabled: 1 }, | ||
| limit: 20 | ||
| }} | ||
| rowKey="name" | ||
| /> | ||
| \`\`\` | ||
|
|
||
| ## Component Props | ||
| - \`doctype\`: Frappe DocType name (e.g., 'User', 'Employee') | ||
| - \`columns\`: Column configuration array | ||
| - \`params\`: Query parameters (fields, filters, orderBy, limit, offset) | ||
| - \`rowKey\`: Primary key field for identifying rows | ||
| - \`auto\`: Auto-fetch on mount (default: true) | ||
| - \`loadingComponent\`: Custom loading UI component | ||
| - \`emptyComponent\`: Custom empty state component | ||
| - \`errorComponent\`: Custom error component | ||
| - \`onDataLoad\`: Callback when data loads | ||
| - \`onError\`: Callback on error | ||
| `, | ||
| }, | ||
| }, | ||
| }, | ||
| tags: ["autodocs"], | ||
| }; | ||
|
|
||
| export default meta; | ||
| type Story = StoryObj<typeof meta>; | ||
|
|
||
| const columns = [ | ||
| { | ||
| label: "Name", | ||
| key: "name", | ||
| width: 3, | ||
| getLabel: ({ row }: { row: Record<string, unknown> }) => row.name, | ||
| prefix: ({ row }: { row: Record<string, unknown> }) => ( | ||
| <Avatar | ||
| shape="circle" | ||
| image={row.user_image as string | undefined} | ||
| size="sm" | ||
| label={row.name as string} | ||
| /> | ||
| ), | ||
| }, | ||
| { | ||
| label: "Email", | ||
| key: "email", | ||
| width: "200px", | ||
| }, | ||
| { | ||
| label: "Type", | ||
| key: "user_type", | ||
| }, | ||
| { | ||
| label: "Enabled", | ||
| key: "enabled", | ||
| }, | ||
| ]; | ||
|
|
||
| const rows = [ | ||
| { | ||
| name: "John Doe", | ||
| email: "john@example.com", | ||
| user_type: "System User", | ||
| enabled: 1, | ||
| user_image: "https://i.pravatar.cc/150?img=1", | ||
| }, | ||
| { | ||
| name: "Jane Smith", | ||
| email: "jane@example.com", | ||
| user_type: "System User", | ||
| enabled: 1, | ||
| user_image: "https://i.pravatar.cc/150?img=2", | ||
| }, | ||
| { | ||
| name: "Bob Johnson", | ||
| email: "bob@example.com", | ||
| user_type: "Website User", | ||
| enabled: 0, | ||
| user_image: "https://i.pravatar.cc/150?img=3", | ||
| }, | ||
| ]; | ||
|
|
||
| export const Default: Story = { | ||
| render: (args) => ( | ||
| <ListView | ||
| columns={columns} | ||
| rows={args.rows as Array<Record<string, unknown>>} | ||
| rowKey={args.rowKey as string} | ||
| options={args.options} | ||
| /> | ||
| ), | ||
| args: { | ||
| rows, | ||
| rowKey: "name", | ||
| options: { | ||
| options: { | ||
| selectable: true, | ||
| }, | ||
| }, | ||
| }, | ||
| }; | ||
|
|
||
| export const Loading: Story = { | ||
| render: () => ( | ||
| <div className="flex items-center justify-center py-12"> | ||
| <div className="text-center"> | ||
| <div className="inline-block animate-spin rounded-full h-8 w-8 border-b-2 border-blue-600" /> | ||
| <p className="mt-3 text-gray-600">Loading records...</p> | ||
| </div> | ||
| </div> | ||
| ), | ||
| }; | ||
|
|
||
| export const WithBadges: Story = { | ||
| render: (args) => { | ||
| const columnsWithBadges = [ | ||
| { | ||
| label: "Name", | ||
| key: "name", | ||
| width: 3, | ||
| getLabel: ({ row }: { row: Record<string, unknown> }) => row.name, | ||
| prefix: ({ row }: { row: Record<string, unknown> }) => ( | ||
| <Avatar | ||
| shape="circle" | ||
| image={row.user_image as string | undefined} | ||
| size="sm" | ||
| label={row.name as string} | ||
| /> | ||
| ), | ||
| }, | ||
| { | ||
| label: "Email", | ||
| key: "email", | ||
| width: "200px", | ||
| }, | ||
| { | ||
| label: "Type", | ||
| key: "user_type", | ||
| }, | ||
| { | ||
| label: "Status", | ||
| key: "enabled", | ||
| getLabel: ({ row }: { row: Record<string, unknown> }) => ( | ||
| <span | ||
| className={`px-2 py-1 rounded text-xs font-medium ${ | ||
| row.enabled | ||
| ? "bg-green-100 text-green-800" | ||
| : "bg-gray-100 text-gray-800" | ||
| }`} | ||
| > | ||
| {row.enabled ? "Active" : "Inactive"} | ||
| </span> | ||
| ), | ||
| }, | ||
| ]; | ||
|
|
||
| return ( | ||
| <ListView | ||
| columns={columnsWithBadges} | ||
| rows={args.rows as Array<Record<string, unknown>>} | ||
| rowKey={args.rowKey as string} | ||
| options={args.options} | ||
| /> | ||
| ); | ||
| }, | ||
| args: { | ||
| rows, | ||
| rowKey: "name", | ||
| options: { | ||
| options: { | ||
| selectable: true, | ||
| }, | ||
| }, | ||
| }, | ||
| }; | ||
|
|
||
| export const Empty: Story = { | ||
| render: () => ( | ||
| <div className="text-center py-12 bg-gray-50 rounded-lg"> | ||
| <svg | ||
| className="mx-auto h-12 w-12 text-gray-400" | ||
| fill="none" | ||
| viewBox="0 0 24 24" | ||
| stroke="currentColor" | ||
| > | ||
| <path | ||
| strokeLinecap="round" | ||
| strokeLinejoin="round" | ||
| strokeWidth={2} | ||
| d="M12 4.354a4 4 0 110 5.292M15 21H3v-1a6 6 0 0112 0v1zm0 0h6v-1a6 6 0 00-9-5.197M13 7a4 4 0 11-8 0 4 4 0 018 0z" | ||
| /> | ||
| </svg> | ||
| <p className="text-gray-500 text-lg mt-4">No records found</p> | ||
| <p className="text-gray-400 text-sm mt-2">Try adjusting your filters</p> | ||
| </div> | ||
| ), | ||
| }; |
There was a problem hiding this comment.
The stories are using ListView component directly instead of the DocTypeTable component that is being introduced in this PR. These stories should demonstrate the DocTypeTable component with various configurations, not just the underlying ListView. This makes the stories misleading as they don't actually test or showcase the new component.
| columns={columns} | ||
| params={{ | ||
| fields: ['name', 'email', 'user_type'], | ||
| orderBy: '`tabUser`.`name` asc', |
There was a problem hiding this comment.
Similar to the table above, the backticks are escaped in the orderBy example. This should show the actual syntax that would be used in code.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…ist.ts Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…ble.tsx Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…ble.tsx Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Add
DocTypeTablecomponent for rendering DocType listsDescription
DocTypeTablecomponent that composesuseFrappeGetDocListwith the existing table/ListView.fields,filters,orderBy,limit,offset) are passed transparently.Usage
Testing
npm run storybookCloses #75
Preview